Fix the logic for CustomSchemeDetector to comply with the documentation. #12
Conversation
bjiang7
left a comment
bjiang7
left a comment
There was a problem hiding this comment.
Thanks for making this fix, Olivier!
| import com.android.utils.childrenIterator | ||
| import org.w3c.dom.Element | ||
|
|
||
| /** |
There was a problem hiding this comment.
Nit: Could you write some more up-to-date KDoc then instead of just removing it?
| context.report(incident) | ||
| private fun hasViewAction(element: Element): Boolean { | ||
| val actions = element.getElementsByTagName(TAG_ACTION) | ||
| for (i in 0 until actions.length) { |
There was a problem hiding this comment.
Optional suggestion: you could use instead a higher-order function like filter. Or instead, just do a for each loop since you don't care about the actual index of the element, i.e. for (action in actions) {...}
https://kotlinlang.org/docs/coding-conventions.html#loops
Please fix throughout.
| /** | ||
| * Detector flagging whether the application has issues verifying custom schemes (i.e. not http/https/file/ftp/ftps). | ||
| */ | ||
| class CustomSchemeDetector : Detector(), XmlScanner { |
There was a problem hiding this comment.
Should we rename this class since it's only checking http/https intent filters not custom schemes?
| return | ||
| } | ||
|
|
||
| for (i in 0 until dataElements.length) { |
There was a problem hiding this comment.
for (element in dataElements) {...}? Unless you plan on using the index for something, it's probably easier to just reference the elements directly in the for loop. Please fix throughout.
| } | ||
|
|
||
| val dataElements = element.getElementsByTagName(TAG_DATA) | ||
| if (dataElements.length == 0) { |
There was a problem hiding this comment.
Do you even need this? Since you'll be iterating over dataElements, if dataElements is empty, it should skip any code in the for loop, right?
| val attribute = child.attributes.item(i) | ||
| val name = attribute.localName ?: continue | ||
| val value = attribute.nodeValue | ||
| private fun hasBrowsableCategory(element: Element): Boolean { |
There was a problem hiding this comment.
You could instead just have one function that takes in the category as a parameter, e.g. hasCategory(element: Element, category: String) and then reuse it for checking android.intent.category.BROWSABLE and android.intent.category.DEFAULT.
2 participants
Modification of the CustomSchemeDetector logic to comly with https://developer.android.com/training/app-links/verify-applinks.